Fix (properly) the logic around prompt re-use & Host Command handling #770

bew · 2024-03-12T06:57:21Z

First PR for nushell/reedline \o/

Revert/Rewrites #758

Fixes #755 (correctly this time 😬)

Where did the newline actually come from in that issue?

The phantom newline that OP reported was due to this block in painter initialization:
(👉 It would detect some text before the cursor and always make a new prompt on the next line..)

reedline/src/painting/painter.rs

Lines 107 to 120 in 0698712

    
           // Assumption: if the cursor is not on the zeroth column, 
        
           // there is content we want to leave intact, thus advance to the next row 
        
           let new_row = if column > 0 { row + 1 } else { row }; 
        
           //  If we are on the last line and would move beyond the last line due to 
        
           //  the condition above, we need to make room for the prompt. 
        
           //  Otherwise printing the prompt would scroll of the stored prompt 
        
           //  origin, causing issues after repaints. 
        
           let new_row = if new_row == self.screen_height() { 
        
               self.print_crlf()?; 
        
               new_row.saturating_sub(1) 
        
           } else { 
        
               new_row 
        
           }; 
        
           self.prompt_start_row = new_row;

Fixes #771 (my issue)
Now I have the same experience than on my zsh config 😃

Supports everything I mentioned in #771
Supports replacing the prompt (even from multi-line old prompt)

Demo

nushell.fixed.prompt.mov

MISSING (DONE)

Unit tests for the painter initialization logic
Refactor painter initialization logic (to remove that unwrap 👀)

What do you think of this implementation?

bew · 2024-03-12T07:29:52Z

This example isn't perfect DX/UX, there's a bad case with a multi-line prompt + cursor on one of the first lines + executing command echo; foo-that-outputs..
Like with a 3 lines prompt, cursor on first line, that command would go down 1 line but would overwrite the 3rd line.

A solution would be to use the command commandline set-cursor --end; echo; foo-that-outputs.
But I'm thinking we could remove the echo and expose a new flag like commandline set-cursor --below-prompt to place the cursor below all the lines of the prompt (and it's nice & easy to read 🙃)

bew · 2024-04-14T10:39:24Z

Hello!
I finally went back to finish this PR, it should be ready for review now 🙏

fdncred · 2024-04-14T13:39:58Z

I've played a little bit with this without nushell and it appears to be outputting less ansi escape codes, which is good.

Separately, on my own and unrelated to this PR, I'm trying to figure out how/why reedline prints so many CRLFs in nushell and also how/why the prompt is redrawn with every character typed.

bew · 2024-04-22T11:37:45Z

Hello o/
Is there anything else I can do to get this PR merged? 🤔

fdncred · 2024-04-23T12:21:53Z

there's a bad case with a multi-line prompt + cursor on one of the first lines + executing command echo; foo-that-outputs..

@bew does this 'bad case' still exist?

bew · 2024-04-23T14:58:54Z

there's a bad case with a multi-line prompt + cursor on one of the first lines + executing command echo; foo-that-outputs..

@bew does this 'bad case' still exist?

The 'bad case' I mentioned is only a limitation with the example, due to using 'echo' in the keybind.
There is nothing I can do in this PR afaik.

To property solve this limitation with using echo to move cursor below the prompt I suggested the creation of a commandline set-cursor option that would do that, but I think this should be done in a separate PR.

fdncred · 2024-04-23T15:13:29Z

ok, thanks. let's try it because we're a week away from a release. if we don't try it now, it'll have to wait until after the release.

Fix (properly) the logic around prompt re-use & Host Command handling

34e06e3

bew added 2 commits April 14, 2024 12:14

Move to dedicated selector function, add tests

413937a

cargo fmt

f1f7ebe

fdncred merged commit 455b9a3 into nushell:main Apr 23, 2024
6 checks passed

bew deleted the fix/prompt-suspend-and-reuse branch April 23, 2024 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix (properly) the logic around prompt re-use & Host Command handling #770

Fix (properly) the logic around prompt re-use & Host Command handling #770

bew commented Mar 12, 2024 •

edited

Loading

bew commented Mar 12, 2024 •

edited

Loading

bew commented Apr 14, 2024

fdncred commented Apr 14, 2024

bew commented Apr 22, 2024 •

edited

Loading

fdncred commented Apr 23, 2024

bew commented Apr 23, 2024

fdncred commented Apr 23, 2024

	// Assumption: if the cursor is not on the zeroth column,
	// there is content we want to leave intact, thus advance to the next row
	let new_row = if column > 0 { row + 1 } else { row };
	// If we are on the last line and would move beyond the last line due to
	// the condition above, we need to make room for the prompt.
	// Otherwise printing the prompt would scroll of the stored prompt
	// origin, causing issues after repaints.
	let new_row = if new_row == self.screen_height() {
	self.print_crlf()?;
	new_row.saturating_sub(1)
	} else {
	new_row
	};
	self.prompt_start_row = new_row;

Fix (properly) the logic around prompt re-use & Host Command handling #770

Fix (properly) the logic around prompt re-use & Host Command handling #770

Conversation

bew commented Mar 12, 2024 • edited Loading

Demo

MISSING (DONE)

bew commented Mar 12, 2024 • edited Loading

bew commented Apr 14, 2024

fdncred commented Apr 14, 2024

bew commented Apr 22, 2024 • edited Loading

fdncred commented Apr 23, 2024

bew commented Apr 23, 2024

fdncred commented Apr 23, 2024

bew commented Mar 12, 2024 •

edited

Loading

bew commented Mar 12, 2024 •

edited

Loading

bew commented Apr 22, 2024 •

edited

Loading